Refactor FXIOS-15851 WindowManager tabManager API should return an optional value by mattreaganmozilla · Pull Request #33928 · mozilla-mobile/firefox-ios

mattreaganmozilla · 2026-05-20T21:03:17Z

📜 Tickets

💡 Description

Refactors the tabManager API in our WindowManager adopters to return an optional value. I've kept the Sentry logging in that func in place, so that unexpected or invalid UUIDs should still be visible to us.

Because we're still logging in that func, in most cases early returns are still reasonable and are preferable to us doing something like returning some other window's tabManager (which is very unsafe).

📝 Checklist

I filled in the ticket numbers and a description of my work
I updated the PR name to follow our PR naming guidelines
I ensured unit tests pass and wrote tests for new code
If working on UI, I checked and implemented accessibility (Dynamic Text and VoiceOver)
If adding telemetry, I read the data stewardship requirements and will request a data review
If adding or modifying strings, I read the guidelines and will request a string review from l10n
If needed, I updated documentation and added comments to complex code

mattreaganmozilla · 2026-05-20T21:03:55Z

+cc @lmarceau Since this is ultimately more of a Tab change than an actual WindowManager change.

mobiletest-ci-bot · 2026-05-20T22:05:53Z

	Messages
📖	Project coverage: 42.46%

💪 Quality guardian

3 tests files modified. You're a champion of test coverage! 🚀

🥇 Perfect PR size

Smaller PRs are easier to review. Thanks for making life easy for reviewers! ✨

🙌 Friday high-five

Thanks for pushing us across the finish line this week! 🙌

💬 Description craftsman

Great PR description! Reviewers salute you 🫡

✅ New file code coverage

No new file detected so code coverage gate wasn't ran.

✅ Existing file code coverage

All modified files meet their thresholds.

Client.app: Coverage: 40.96

File	Coverage
TabManagerMiddleware.swift	40.19%	⚠️
WindowManager+DebugUtilities.swift	0.0%	⚠️
SceneDelegate.swift	33.78%	⚠️
StartAtHomeMiddleware.swift	90.0%	✅
NativeErrorPageMiddleware.swift	0.0%	⚠️
TabErrorTelemetryHelper.swift	46.02%	⚠️
TranslationsMiddleware.swift	89.13%	✅
AppDelegate.swift	0.0%	⚠️
ToolbarMiddleware.swift	93.81%	✅
SummarizeMiddleware.swift	97.71%	✅
TranslationsService.swift	37.82%	⚠️
WindowManager.swift	86.74%	✅

Generated by 🚫 Danger Swift against 35f4002

mattreaganmozilla · 2026-05-20T22:23:24Z

-
-    /// Convenience. Provides opportunity for safety checks or window validation.
-    @MainActor
-    func windowExists(uuid: WindowUUID) -> Bool


Removed windowExists() because I think it was a code smell. Generally callers should not need to directly check this.

lmarceau · 2026-05-21T20:43:15Z

        defaults.set(tabCounts, forKey: entryPoint.defaultsKey)
    }

-    /// It's possible for this telemetry helper to be called during onboarding flow before


That isn't a problem anymore (the comment) ?

The tabManagerAvailable func was added as a safety check, but with these refactors that safety check is now baked into the tabManager(for:) func, so each call site is forced to handle the potential nil value. So with this function gone this comment doesn't really have a great place to live now, since we're checking the optional value separately in each call site. We could maybe add the comment to the top of the file if you think it's still worth calling out?

Maybe we can do that yeah, I'll let you decide, you added that comment in the first place so you have more context than me! If you think it's useful to keep then lets move it, if not then 👋

Yeah this was a good call-out thank you; I've moved the comment to the top of the file.

yoanarios

Changes look good just a small nit and unit test request

yoanarios · 2026-05-22T18:51:16Z

    @MainActor
    private func startAtHomeCheck(windowUUID: WindowUUID) -> Bool {
-        let tabManager = windowManager.tabManager(for: windowUUID)
+        guard let tabManager = windowManager.tabManager(for: windowUUID) else { return false}


nit

Suggested change

guard let tabManager = windowManager.tabManager(for: windowUUID) else { return false}

guard let tabManager = windowManager.tabManager(for: windowUUID) else { return false }

Thank you, fixed

yoanarios · 2026-05-22T19:00:33Z

        wrappedManager.newBrowserWindowConfigured(windowInfo, uuid: uuid)
    }

-    func tabManager(for windowUUID: WindowUUID) -> TabManager {


Code wise changes seems right my only ask it will be to add support in the mock to return nil and add at least a test in TabManagerMiddlewareTests that tests the nil value instead of force unwrap to cover that case

I could be confused but isn't that a superfluous test? I don't think that actually exercises any real code in the client, right? LMK if I'm misunderstanding tho, I'm happy to update the unit tests.

Fair point, I agree we don’t want to add a test that only validates the mock. My concern is more about the new API contract: tabManager(for:) now returns an optional, and TabManagerMiddleware is one of the consumers that needs to behave safely when that value is nil.

Right now the production implementation explicitly returns nil for invalid/missing windows instead of falling back to an unsafe tab manager, so I think it’s worth having at least one unit test that exercises a real consumer path with a nil TabManager. That would protect us from future regressions where someone forces unwrapping or assumes the tab manager always exists again.

I've pushed up unit tests in 35f4002 but LMK if you had something else specific in mind.

yoanarios

LGTM 🚀

github-actions · 2026-05-26T21:29:40Z

🚀 PR merged to main, targeting version: 151.3

…tional value (mozilla-mobile#33928) * [FXIOS-15851] Refactor tabManager(for:) to return an optional value * Unit test updates * [FXIOS-15851] Comments * Spacing * [FXIOS-15851] Add unit tests

[FXIOS-15851] Refactor tabManager(for:) to return an optional value

e169e1e

mattreaganmozilla requested a review from a team as a code owner May 20, 2026 21:03

mattreaganmozilla requested a review from yoanarios May 20, 2026 21:03

Unit test updates

ec838e8

mattreaganmozilla commented May 20, 2026

View reviewed changes

mattreaganmozilla requested a review from lmarceau May 21, 2026 19:04

lmarceau reviewed May 21, 2026

View reviewed changes

lmarceau self-requested a review May 21, 2026 20:43

lmarceau approved these changes May 22, 2026

View reviewed changes

mattreaganmozilla removed the request for review from yoanarios May 22, 2026 18:13

[FXIOS-15851] Comments

e052387

mattreaganmozilla force-pushed the mr/15851 branch from 62b158b to e052387 Compare May 22, 2026 18:36

yoanarios reviewed May 22, 2026

View reviewed changes

mattreaganmozilla added 2 commits May 22, 2026 12:14

Spacing

1290660

[FXIOS-15851] Add unit tests

35f4002

yoanarios approved these changes May 26, 2026

View reviewed changes

mattreaganmozilla merged commit cb8b14a into mozilla-mobile:main May 26, 2026
9 checks passed

	guard let tabManager = windowManager.tabManager(for: windowUUID) else { return false}
	guard let tabManager = windowManager.tabManager(for: windowUUID) else { return false }

Conversation

mattreaganmozilla commented May 20, 2026

📜 Tickets

💡 Description

📝 Checklist

Uh oh!

mattreaganmozilla commented May 20, 2026

Uh oh!

mobiletest-ci-bot commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💪 Quality guardian

🥇 Perfect PR size

🙌 Friday high-five

💬 Description craftsman

✅ New file code coverage

✅ Existing file code coverage

Client.app: Coverage: 40.96

Uh oh!

mattreaganmozilla May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattreaganmozilla May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yoanarios left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yoanarios left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented May 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mobiletest-ci-bot commented May 20, 2026 •

edited

Loading

mattreaganmozilla May 20, 2026 •

edited

Loading

mattreaganmozilla May 21, 2026 •

edited

Loading